feat(publish): allow async custom Ajv keywords#2749
Open
fpotier wants to merge 2 commits into
Open
Conversation
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The
Keywordtype currently declaresvalidateasobject, but you treat it as a function inresolveFunction; tightening this type (e.g. tounknownor a function signature) would better match usage and avoid accidentally passing non-callable values. - Calling
this.ajv.removeSchema()without arguments after every validation clears all compiled schemas, which can impact concurrent or subsequent validations; consider removing only the specific schema that was compiled or reusing a compiled schema instead of wiping the entire registry.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `Keyword` type currently declares `validate` as `object`, but you treat it as a function in `resolveFunction`; tightening this type (e.g. to `unknown` or a function signature) would better match usage and avoid accidentally passing non-callable values.
- Calling `this.ajv.removeSchema()` without arguments after every validation clears all compiled schemas, which can impact concurrent or subsequent validations; consider removing only the specific schema that was compiled or reusing a compiled schema instead of wiping the entire registry.
## Individual Comments
### Comment 1
<location path="src/published-data/validator.service.ts" line_range="33" />
<code_context>
"findOne" | "findAll" | "count"
>;
+type Keyword = { keyword: string; validate: object };
+
@Injectable()
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Use a more precise type for `validate` on `Keyword` to avoid losing type-safety.
Typing `validate` as `object` removes any guarantee that it’s callable, even though it’s later treated as a function (e.g. passed to `resolveFunction`, cast to `KeywordDefinition`). Please narrow this to a callable type such as `((...args: any[]) => any) | AsyncKeywordValidateFunction` (or at least `unknown`) so non-callable values are caught at compile time.
```suggestion
type Keyword = {
keyword: string;
validate: Exclude<KeywordDefinition["validate"], undefined>;
};
```
</issue_to_address>
### Comment 2
<location path="src/published-data/validator.service.ts" line_range="105" />
<code_context>
const validateFn = this.ajv.compile(this.config.metadataSchema as Schema);
validateFn(publishedData.metadata);
+ this.ajv.removeSchema();
return validateFn.errors;
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Calling `removeSchema()` without a key can cause concurrency issues and unnecessary work.
`removeSchema()` with no arguments clears all schemas on the shared Ajv instance. Because `validate` is `async` and awaits `loadDynamicFunctions`, concurrent calls can interleave so that one call deletes schemas another still needs. It also prevents Ajv from reusing compiled schemas, adding validation overhead. Prefer removing only the relevant schema by key/ref, or creating a new Ajv instance per validation rather than mutating a shared one.
</issue_to_address>
### Comment 3
<location path="src/published-data/validator.service.ts" line_range="117" />
<code_context>
}
- private async loadDynamicDefaultFunctions(
+ private async loadDynamicFunctions(
publishedData:
| CreatePublishedDataV4Dto
</code_context>
<issue_to_address>
**issue (complexity):** Consider splitting dynamic defaults and keyword handling into separate, domain-specific loaders with dedicated async-to-sync resolvers to simplify the control flow and remove generic branching logic.
You can reduce the new complexity without changing behavior by making the async/sync normalization more domain‑specific and pushing the branching into focused helpers.
### 1. Split `loadDynamicFunctions` into two focused methods
Right now `loadDynamicFunctions` handles both dynamic defaults and keywords, plus generic resolution. You can preserve behavior but separate concerns:
```ts
async validate(publishedData: CreatePublishedDataV4Dto | UpdatePublishedDataV4Dto | PartialUpdatePublishedDataV4Dto) {
if (isNil(this.config.metadataSchema)) {
return null;
}
const context = {
publishedData,
proposalService: this.proposalsService as ReadOnlyProposalsService,
datasetsService: this.datasetsService as ReadOnlyDatasetsService,
attachmentsService: this.attachmentsService as ReadOnlyAttachmentsService,
};
await this.loadDynamicDefaults(context);
await this.loadKeywords(context);
const validateFn = this.ajv.compile(this.config.metadataSchema as Schema);
validateFn(publishedData.metadata);
this.ajv.removeSchema();
return validateFn.errors;
}
```
Then:
```ts
private async loadDynamicDefaults(context: {
publishedData: CreatePublishedDataV4Dto | UpdatePublishedDataV4Dto | PartialUpdatePublishedDataV4Dto;
proposalService: ReadOnlyProposalsService;
datasetsService: ReadOnlyDatasetsService;
attachmentsService: ReadOnlyAttachmentsService;
}) {
for (const [name, implementation] of this.dynamicDefaults.entries()) {
const resolved = await this.resolveDynamicDefault(name, implementation, context);
if (!resolved) continue;
def.DEFAULTS[name] = resolved;
}
}
private async loadKeywords(context: /* same type as above */) {
for (const keywordDefinition of this.keywords) {
const resolved = await this.resolveKeywordValidate(keywordDefinition, context);
if (!resolved) continue;
this.overwriteKeyword(resolved);
}
}
```
This keeps the “two entry points” requested by the reviewer while reusing the same context object.
### 2. Replace generic `resolveFunction` with domain‑specific resolvers
You can keep all behavior but hide the async/sync branching from call sites and avoid `constructor.name` checks there.
```ts
private async resolveDynamicDefault(
name: string,
implementation: unknown,
context: unknown,
): Promise<DynamicDefaultFunc | null> {
if (typeof implementation !== "function") {
Logger.error(
`Ignoring dynamicDefaults function '${name}' should be of type 'function' not '${typeof implementation}'.`,
);
return null;
}
if (implementation.constructor.name === "AsyncFunction") {
try {
const syncFunc = await implementation(context);
// Always return a sync DynamicDefaultFunc
return () => syncFunc;
} catch (err) {
throw new Error(
`Executing dynamicDefaults function '${name}' failed with the following error:`,
{ cause: err },
);
}
}
// Already a sync DynamicDefaultFunc
return implementation as DynamicDefaultFunc;
}
private async resolveKeywordValidate(
keywordDefinition: Keyword,
context: unknown,
): Promise<Keyword | null> {
const { keyword, validate } = keywordDefinition;
if (typeof validate !== "function") {
Logger.error(
`Ignoring keyword '${keyword}' validate should be of type 'function' not '${typeof validate}'.`,
);
return null;
}
if (validate.constructor.name === "AsyncFunction") {
try {
const resolvedValidate = await validate(context);
// Normalize to a sync validate function
const normalized: Keyword = {
...keywordDefinition,
validate: resolvedValidate,
};
return normalized;
} catch (err) {
throw new Error(
`Executing keyword '${keyword}' failed with the following error:`,
{ cause: err },
);
}
}
// Already sync
return keywordDefinition;
}
```
With this, `loadDynamicDefaults` and `loadKeywords` no longer need to:
- Check `constructor.name` themselves.
- Decide whether to wrap with `() => resolved` or not.
- Know whether they’re getting a function or a pre‑evaluated value.
The async/sync distinction is normalized “at the boundary”, as the reviewer suggested.
### 3. Keep `overwriteKeyword` simple and tightly scoped
You can leave `overwriteKeyword` as is, but since `resolveKeywordValidate` now guarantees a normalized `Keyword` with a sync `validate`, its responsibility is purely “ensure the latest definition is registered”:
```ts
private overwriteKeyword(keywordDefinition: Keyword) {
if (this.ajv.getKeyword(keywordDefinition.keyword)) {
this.ajv.removeKeyword(keywordDefinition.keyword);
}
this.ajv.addKeyword(keywordDefinition as KeywordDefinition);
}
```
Because async behavior is already flattened by `resolveKeywordValidate`, this method no longer participates in the async/sync distinction, which simplifies its contract.
---
These changes:
- Keep all current functionality (including pre‑executing async functions using `context`).
- Reduce branching at call sites.
- Remove the polymorphic `resolveFunction` and use explicit, domain‑specific helpers.
- Make the control flow clearer by separating “dynamic defaults” and “keywords” into two small, focused loaders.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Inject context into keywords if the
validatefunction is defined async.Motivation
At PSI we have multiple metadata fields that:
Using an ajv keyword instead of a dynamic default has two advantage for us:
Tests included
Documentation
official documentation info
Summary by Sourcery
Support injecting context into dynamic Ajv extensions and reuse them for both dynamic defaults and custom keywords, including async implementations.
New Features:
Enhancements:
Documentation:
Tests: